Skip to content

fix: intermittent incorrect logging of CheckQueue for invalid blocks#7312

Open
knst wants to merge 1 commit intodashpay:developfrom
knst:fix-checkqueue-error-code
Open

fix: intermittent incorrect logging of CheckQueue for invalid blocks#7312
knst wants to merge 1 commit intodashpay:developfrom
knst:fix-checkqueue-error-code

Conversation

@knst
Copy link
Copy Markdown
Collaborator

@knst knst commented May 7, 2026

Issue being fixed or feature implemented

ConnectBlock may return no-named failure instead proper error code such as:

2024-09-23T13:13:10Z ERROR: ConnectBlock: CheckQueue failed

What was done?

This PR fixes this intermittent failure

How Has This Been Tested?

N/A

Breaking Changes

N/A

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

@knst
fix: intermittent incorrect logging of CheckQueue for invalid blocks
76e6645 
It helps to return valid error during ConnectBlock instead no-named failure

    2024-09-23T13:13:10Z ERROR: ConnectBlock: CheckQueue failed
@knst knst added this to the 24 milestone May 7, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 7, 2026

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 7, 2026

Review Change Stack
No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4c59195f-c7fe-4a7e-8f63-e19a1143e462

📥 Commits

Reviewing files that changed from the base of the PR and between 5fd84aa and 76e6645.

📒 Files selected for processing (1)
  • src/validation.cpp

Walkthrough

The pull request modifies CChainState::ConnectBlock() in src/validation.cpp to reorder memory allocations. The std::vector<PrecomputedTransactionData> txsdata vector is now allocated before the CCheckQueueControl<CScriptCheck> control object is constructed, rather than after. This ensures that the transaction data vector exists prior to control initialization, preventing potential allocation-related side effects during script-check worker execution.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly addresses the main fix: ensuring proper error logging for CheckQueue failures in ConnectBlock, which matches the core change of moving txsdata allocation to prevent intermittent errors.
Description check ✅ Passed The description clearly identifies the issue (intermittent CheckQueue failures with generic logging) and explains that the PR fixes this problem, which aligns with the changeset's objective of preventing CheckQueue execution issues.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@thepastaclaw
Copy link
Copy Markdown

thepastaclaw commented May 7, 2026 •
edited
Loading

✅ Review complete (commit 76e6645)

@knst knst requested review from PastaPastaPasta and UdjinM6 May 7, 2026 18:16
thepastaclaw
thepastaclaw reviewed May 7, 2026
View reviewed changes
Copy link
Copy Markdown

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

I reviewed the exact queued head 76e6645cd43e73847e1444bb1aef788f58e8c0cf and only src/validation.cpp changes in this PR. The change is a narrowly scoped fix to object lifetime ordering in CChainState::ConnectBlock: txsdata is now guaranteed to outlive CCheckQueueControl, which matches the existing comment and prevents queued CScriptCheck workers from observing destroyed PrecomputedTransactionData. I did not find any correctness, consensus, or Dash-specific interaction issues in the current patch.

Reviewed commit: 76e6645

thepastaclaw
thepastaclaw reviewed May 8, 2026
View reviewed changes
Copy link
Copy Markdown

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The single-line reordering in src/validation.cpp ensures txsdata is constructed before CCheckQueueControl<CScriptCheck> control and therefore destroyed after it, matching the in-source comment that txsdata must remain valid until control's destructor finishes running queued script checks. The fix is narrow, correct, and has no Dash subsystem interaction.

Reviewed commit: 76e6645

Fresh dispatcher run for this queue item. A same-SHA review already existed, so this records the fresh verification without duplicating inline threads.

Code Review

Reviewed commit: 76e6645

UdjinM6
UdjinM6 approved these changes May 9, 2026
View reviewed changes
Copy link
Copy Markdown

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 76e6645

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@PastaPastaPasta PastaPastaPasta Awaiting requested review from PastaPastaPasta

+1 more reviewer

@thepastaclaw thepastaclaw thepastaclaw left review comments

Reviewers whose approvals may not affect merge requirements

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

24

Development

Successfully merging this pull request may close these issues.

3 participants

@knst @thepastaclaw @UdjinM6